Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Extract readonly method #37524

Merged
merged 7 commits into from
Aug 11, 2019
Merged

Extract readonly method #37524

merged 7 commits into from
Aug 11, 2019

Conversation

RikkiGibson
Copy link
Contributor

Resolves #34647

@RikkiGibson RikkiGibson added Area-IDE IDE-CodeStyle Built-in analyzers, fixes, and refactorings labels Jul 28, 2019
@RikkiGibson RikkiGibson marked this pull request as ready for review July 29, 2019 00:22
@RikkiGibson RikkiGibson requested review from a team as code owners July 29, 2019 00:22
@RikkiGibson
Copy link
Contributor Author

Could I please get a review from @dotnet/roslyn-ide

@RikkiGibson
Copy link
Contributor Author

Ping @dotnet/roslyn-ide

@RikkiGibson RikkiGibson force-pushed the extract-readonly-method branch from bcc9b27 to 1770001 Compare August 1, 2019 20:30
@RikkiGibson
Copy link
Contributor Author

Seems like I somehow got some 16.4-p1 commits in here by accident so I had to force push to narrow it down to just the commits that actually contain my work. Sorry about that.

genlu
genlu reviewed Aug 5, 2019
View reviewed changes
src/Features/Core/Portable/ExtractMethod/MethodExtractor.Analyzer.cs
var instanceMemberIsUsed = thisParameterBeingRead != null || isThisParameterWritten;
var shouldBeReadOnly = !isThisParameterWritten
&& thisParameterBeingRead != null
&& thisParameterBeingRead.Type is { TypeKind: TypeKind.Struct, IsReadOnly: false };
Copy link
Member

@genlu genlu Aug 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why do we have IsReadOnly: false here. Shouldn't the new method be made readonly when the type of this is readonly struct?

nvm, I got it now.

genlu
genlu approved these changes Aug 5, 2019
View reviewed changes
Copy link
Member

@genlu genlu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDE change LGTM.

@RikkiGibson RikkiGibson changed the base branch from master to release/dev16.4-preview1 August 5, 2019 23:40
@RikkiGibson RikkiGibson requested a review from a team August 5, 2019 23:41
@RikkiGibson
Copy link
Contributor Author

A second compiler team review is needed, and I need to target 16.4 due to the flow analysis changes in this PR.

@jcouv
Copy link
Member

jcouv commented Aug 9, 2019

Looking

jcouv
jcouv approved these changes Aug 9, 2019
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compiler change LGTM Thanks (iteration 7)

@jcouv jcouv added this to the 16.4 milestone Aug 9, 2019
@jcouv jcouv self-assigned this Aug 9, 2019
@RikkiGibson RikkiGibson merged commit 60409f6 into dotnet:release/dev16.4-preview1 Aug 11, 2019
@RikkiGibson RikkiGibson deleted the extract-readonly-method branch August 11, 2019 19:03
@jcouv jcouv modified the milestones: 16.4, 16.4.P1 Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@jaredpar jaredpar jaredpar left review comments

@jcouv jcouv jcouv approved these changes

@cston cston cston approved these changes

@genlu genlu genlu approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers Area-IDE IDE-CodeStyle Built-in analyzers, fixes, and refactorings
Projects
None yet
Milestone
16.4.P1
Development

Successfully merging this pull request may close these issues.
6 participants
@RikkiGibson @jcouv @jaredpar @genlu @CyrusNajmabadi @cston